Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: use browser :focus-visible if supported #24195

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

ling1726
Copy link
Member

@ling1726 ling1726 commented Aug 3, 2022

Current Behavior

v9 :focus-visible ponyfill will always be used

New Behavior

Both native :focus-visible and ponyfill styles are written to DOM, but if the ponyfill is never applied, its styles will never be used because there is no selector

:focus-visible native

Changes to :focus-visible will not trigger :focus-within

time elapsed fast reject count match attempts match count
116 0 783 265

:focus-visible ponyfill 🦄

time elapsed fast reject count match attempts match count
377 0 1484 364

:focus-visible Improvement in %

time elapsed fast reject count match attempts match count
69.2% 0% 47.2% 27.2%

Related Issue(s)

Addresses #24183

@github-actions github-actions bot added this to the July Project Cycle Q3 2022 milestone Aug 3, 2022
@ling1726 ling1726 marked this pull request as ready for review August 3, 2022 09:39
@ling1726 ling1726 requested a review from a team as a code owner August 3, 2022 09:39
@codesandbox-ci
Copy link

codesandbox-ci bot commented Aug 3, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit bc2f8cb:

Sandbox Source
@fluentui/react 8 starter Configuration
@fluentui/react-components 9 starter Configuration

@fabricteam
Copy link
Collaborator

fabricteam commented Aug 3, 2022

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-accordion
Accordion (including children components)
79.485 kB
24.082 kB
81.816 kB
24.665 kB
2.331 kB
583 B
react-alert
Alert
82.803 kB
20.582 kB
86.055 kB
21.271 kB
3.252 kB
689 B
react-avatar
AvatarGroup
139.268 kB
41.173 kB
140.849 kB
41.539 kB
1.581 kB
366 B
react-button
Button
36.396 kB
9.575 kB
39.648 kB
10.24 kB
3.252 kB
665 B
react-button
CompoundButton
43.469 kB
10.812 kB
46.721 kB
11.474 kB
3.252 kB
662 B
react-button
MenuButton
39.014 kB
10.456 kB
42.266 kB
11.116 kB
3.252 kB
660 B
react-button
SplitButton
46.506 kB
11.827 kB
50.148 kB
12.65 kB
3.642 kB
823 B
react-button
ToggleButton
51.912 kB
11.004 kB
56.472 kB
11.968 kB
4.56 kB
964 B
react-card
Card - All
67.42 kB
19.249 kB
69.936 kB
19.83 kB
2.516 kB
581 B
react-card
Card
63.102 kB
18.167 kB
65.618 kB
18.747 kB
2.516 kB
580 B
react-combobox
Combobox (including child components)
75.081 kB
24.145 kB
75.123 kB
24.166 kB
42 B
21 B
react-combobox
Dropdown (including child components)
74.594 kB
24.134 kB
74.636 kB
24.155 kB
42 B
21 B
react-components
react-components: Accordion, Button, FluentProvider, Image, Menu, Popover
192.699 kB
52.786 kB
200.655 kB
54.926 kB
7.956 kB
2.14 kB
react-components
react-components: FluentProvider & webLightTheme
32.688 kB
10.736 kB
32.73 kB
10.757 kB
42 B
21 B
react-dialog
Dialog (including children components)
85.049 kB
25.293 kB
87.422 kB
25.926 kB
2.373 kB
633 B
react-link
Link
12.197 kB
4.912 kB
12.452 kB
4.976 kB
255 B
64 B
react-menu
Menu (including children components)
118.866 kB
36.006 kB
121.239 kB
36.69 kB
2.373 kB
684 B
react-menu
Menu (including selectable components)
122.065 kB
36.499 kB
124.438 kB
37.195 kB
2.373 kB
696 B
react-popover
Popover
106.133 kB
32.244 kB
106.175 kB
32.264 kB
42 B
20 B
react-portal
Portal
10.49 kB
3.845 kB
10.532 kB
3.867 kB
42 B
22 B
react-provider
FluentProvider
15.565 kB
5.818 kB
15.607 kB
5.837 kB
42 B
19 B
react-tooltip
Tooltip
45.509 kB
15.537 kB
45.551 kB
15.558 kB
42 B
21 B
Unchanged fixtures
Package & Exports Size (minified/GZIP)
react-avatar
Avatar
48.533 kB
13.78 kB
react-avatar
AvatarGroupItem
68.248 kB
19.109 kB
react-card
CardFooter
8.461 kB
3.555 kB
react-card
CardHeader
9.504 kB
3.896 kB
react-card
CardPreview
8.562 kB
3.61 kB
react-radio
Radio
36.329 kB
12.024 kB
react-radio
RadioGroup
14.319 kB
5.711 kB
react-slider
Slider
31.988 kB
10.019 kB
react-switch
Switch
32.781 kB
10.348 kB
🤖 This report was generated against a0e1e0c2780ad6ba73aaeee1aa1525168a4d4045

@fabricteam
Copy link
Collaborator

fabricteam commented Aug 3, 2022

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1598 1518 5000
Button mount 1152 1140 5000
FluentProvider mount 1864 1871 5000
FluentProviderWithTheme mount 730 741 10
FluentProviderWithTheme virtual-rerender 729 690 10
FluentProviderWithTheme virtual-rerender-with-unmount 734 755 10
MakeStyles mount 2247 2351 50000
SpinButton mount 3107 3093 5000

@size-auditor
Copy link

size-auditor bot commented Aug 3, 2022

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: a0e1e0c2780ad6ba73aaeee1aa1525168a4d4045 (build)

Copy link
Contributor

@bsunderhus bsunderhus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -103,3 +102,7 @@ function alreadyInScope(el: HTMLElement | null | undefined): boolean {

return alreadyInScope(el?.parentElement);
}

function browserSupportsFocusVisible() {
return CSS.supports('selector(:focus-visible)');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to hear from @mshoho that we can use focus-visible in supported browsers.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@miroslavstastny focus-visible isn't fully sufficient. It cannot be triggered programmatically. And keyborg has things like detecting if the focus is moved by screen readers (and consequently enabling the keyboard navigation mode).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -28,6 +28,8 @@ export const createCustomFocusIndicatorStyle = (
[`:global(.fui-FluentProvider)`]: {
[`& .${FOCUS_VISIBLE_CLASS}`]: style,
},
Copy link
Contributor

@jspurlin jspurlin Aug 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be wrapped in a @ supports not selector(:focus-visible) block so that it's easier for the browser to know they don't have to try and match this style if :focus-visible is supported?

Copy link
Contributor

@jspurlin jspurlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Outside of my minor feedback, this PR looks good

@ling1726 ling1726 added the Status: Blocked Resolution blocked by another issue label Aug 10, 2022
@ling1726
Copy link
Member Author

Waiting on microsoft/keyborg#19 so that we can handle edge cases where screen readers pull focus

@bsunderhus bsunderhus self-requested a review November 3, 2022 17:08
@msft-fluent-ui-bot
Copy link
Collaborator

Because this pull request has not had activity for over 150 days, we're automatically closing it for house-keeping purposes.

The pull request will still be available for reference. If it's still relevant to merge at some point, you can reopen or make a new version based on the latest code.

@msft-fluent-ui-bot msft-fluent-ui-bot added the Resolution: Soft Close Soft closing inactive issues over a certain period label Apr 2, 2023
@ling1726 ling1726 reopened this Apr 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: Soft Close Soft closing inactive issues over a certain period Status: Blocked Resolution blocked by another issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants